Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issue #15838

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

fdumontet6WIND
Copy link
Contributor

bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issues

whith the following config

router bgp 65001
no bgp ebgp-requires-policy
neighbor 192.168.1.2 remote-as external
neighbor 192.168.1.2 timers 3 10
!
address-family ipv4 unicast
neighbor 192.168.1.2 route-map r2 in
exit-address-family
exit
!
bgp as-path access-list FIRST seq 5 permit ^65
bgp as-path access-list SECOND seq 5 permit 2$
!
route-map r2 permit 6
match ip address prefix-list p2
set as-path exclude as-path-access-list SECOND
exit
!
route-map r2 permit 10
match ip address prefix-list p1
set as-path exclude 65003
exit
!
route-map r2 permit 20
match ip address prefix-list p3
set as-path exclude all
exit

making some
no bgp as-path access-list SECOND permit 2$
bgp as-path access-list SECOND permit 3$

clear bgp *

no bgp as-path access-list SECOND permit 3$
bgp as-path access-list SECOND permit 2$

clear bgp *

will induce some crashes

thus we rework the links between aslists and aspath_exclude

Signed-off-by: Francois Dumontet francois.dumontet@6wind.com

@frrbot frrbot bot added bgp bugfix tests Topotests, make check, etc labels Apr 25, 2024
bgpd/bgp_aspath.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added the rebase PR needs rebase label Apr 29, 2024
@fdumontet6WIND fdumontet6WIND force-pushed the fix_regexx_exclude branch 4 times, most recently from 3905f36 to 52c04d6 Compare April 30, 2024 08:11
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@ton31337
Copy link
Member

@Mergifyio backport stable/10.0

Copy link

mergify bot commented Apr 30, 2024

backport stable/10.0

✅ Backports have been created

@fdumontet6WIND
Copy link
Contributor Author

ci:rerun

bgpd/bgp_routemap.c Outdated Show resolved Hide resolved
bgpd/bgp_aspath.c Outdated Show resolved Hide resolved
@fdumontet6WIND
Copy link
Contributor Author

ci:rerun

@fdumontet6WIND
Copy link
Contributor Author

ci:rerun

bgpd/bgp_routemap.c Outdated Show resolved Hide resolved
@fdumontet6WIND fdumontet6WIND requested a review from ton31337 May 6, 2024 10:54
@fdumontet6WIND fdumontet6WIND requested a review from riw777 May 13, 2024 08:32
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

bgpd/bgp_aspath.c Outdated Show resolved Hide resolved
…t issues

whith the following config

router bgp 65001
 no bgp ebgp-requires-policy
 neighbor 192.168.1.2 remote-as external
 neighbor 192.168.1.2 timers 3 10
 !
 address-family ipv4 unicast
  neighbor 192.168.1.2 route-map r2 in
 exit-address-family
exit
!
bgp as-path access-list FIRST seq 5 permit ^65
bgp as-path access-list SECOND seq 5 permit 2$
!
route-map r2 permit 6
 match ip address prefix-list p2
 set as-path exclude as-path-access-list SECOND
exit
!
route-map r2 permit 10
 match ip address prefix-list p1
 set as-path exclude 65003
exit
!
route-map r2 permit 20
 match ip address prefix-list p3
 set as-path exclude all
exit

making some
no bgp as-path access-list SECOND permit 2$
bgp as-path access-list SECOND permit 3$

clear bgp *

no bgp as-path access-list SECOND permit 3$
bgp as-path access-list SECOND permit 2$

clear bgp *

will induce some crashes

thus  we rework the links between aslists and aspath_exclude

Signed-off-by: Francois Dumontet <francois.dumontet@6wind.com>
add some match in route map rules
add some set unset bgp access path list
add another prefix for better tests discrimination
update expected results

Signed-off-by:  Francois Dumontet <francois.dumontet@6wind.com>
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


void as_exclude_remove_orphan(struct aspath_exclude *ase)
{
if (as_list_list_count(&as_exclude_list_orphan))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to have this test? Why not just delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orphan list may be destroyed before the as_path_exclude that we are destroying currently.
this occurs at termination time.
this test verify the existence of the list .

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, waiting on comments by other reviewers

@riw777
Copy link
Member

riw777 commented May 28, 2024

I think we need to address @donaldsharp 's comment before merging (?)

@fdumontet6WIND
Copy link
Contributor Author

Are you interested in this fix?

@riw777
Copy link
Member

riw777 commented Jun 4, 2024

Are you interested in this fix?

@donaldsharp just wanted to check it before we merge ...

@ton31337 ton31337 added this to the 10.1 milestone Jun 11, 2024
@riw777 riw777 merged commit 7a87166 into FRRouting:master Jun 24, 2024
9 checks passed
@ton31337
Copy link
Member

@Mergifyio backport dev/10.1

Copy link

mergify bot commented Jun 25, 2024

backport dev/10.1

✅ Backports have been created

riw777 added a commit that referenced this pull request Jun 25, 2024
 bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issue (backport #15838)
riw777 added a commit that referenced this pull request Jun 25, 2024
 bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issue (backport #15838)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport bgp bugfix master rebase PR needs rebase size/L tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants